fix: quote automation commands for the host shell - #700

The "^%" escape only holds on the interactive cmd prompt. Inside a batch file (.bat/.cmd) — arguably the main automation-command destination — percent expansion is an earlier phase with different rules: a lone unmatched % is stripped, and an undefined %...% construct is deleted outright. So:

• 100% Done → "100"^%" Done" → batch reduces it to "100"^" Done" → argv sees 100" + Done
• %PATH% → ""^%"PATH"^%"" → batch deletes the whole %"PATH"^% run

The batch escape is %%, but that doesn't collapse on the interactive line, so no single encoding satisfies both. In neither mode does ^ actually escape % (percent expansion runs before caret processing); interactive survives only because undefined-variable references are left untouched there — which is also the assumption simulateCmd bakes in, so the round-trip test can't see this. Worth promoting % into the "can't be fixed" list above (scoped to batch files), and it's another candidate for the warning you float in open question 5.

Actioned in 102520c (docs) and ad69548 (the warning).

quoteCmd's "can't be fixed" list now has three entries and % is one of them, scoped the way you describe: the caret doesn't escape % in either mode, percent expansion being an earlier parsing phase than caret processing; the interactive prompt survives only because it leaves an unmatched % and an undefined %var% alone, and a batch file drops the former and deletes the latter. The comment also records that %% is the batch escape and doesn't collapse at the prompt, so no single encoding satisfies both and the interactive one is the one worth having.

simulateCmd's doc comment now says outright that it models the interactive prompt and that a value containing % doesn't survive a batch file, so the round trip test no longer reads as if it covered both.

On the warning: pkg/util/shell/warn.go holds a per-shell table of characters a quoted value can't reliably carry, and pkg/util/flag/flag.go appends the result to the generated command. % is in it with batch-specific wording — "is expanded before any escaping is applied, so it only survives at the interactive prompt; a .bat or .cmd script drops an unmatched % and replaces %var%" — alongside ! and a line break. Covered by TestPasteWarning in pkg/util/shell/warn_test.go: cmd warns about a percent (100% Cotton), cmd warns about an environment variable (%PATH%), and bash is always fine / powershell is always fine to pin that the warning is cmd-only.

Unverified by execution: there's no cmd.exe or Windows host available here, so both the prompt and the batch behaviours are reasoned from the documented parsing phases rather than run. TestQuoteCmd_RoundTrip exercises the model, not a real cmd — which is the limitation the comment now admits to rather than hides.

PowerShell's tokenizer treats the Unicode single-quote variants — U+2018 ‘, U+2019 ’, U+201A ‚, U+201B ‛ — as string delimiters equivalent to '. Only ASCII ' is doubled here, so a value containing a curly apostrophe (easy to acquire from a web UI or Word, e.g. Bob’s Project) produces 'Bob’s Project', which pwsh rejects as a parse error.

Verified on pwsh 7: /bin/echo 'Bob’s Project' exits 1 with a parse error, while 'Bob’’s Project' round-trips byte-identically — so doubling all four variants alongside ' fixes it. (isBare already forces quoting for these since they're non-ASCII; the gap is only in the escaping.) Bash and cmd don't treat smart quotes specially, so this is PowerShell-only.

Actioned in 8a88e5f.

All four variants are now doubled alongside the ASCII quote, via a strings.NewReplacer rather than the single ReplaceAll:

var powerShellQuoteEscaper = strings.NewReplacer(
	`'`, `''`,
	"‘", "‘‘", // left single quotation mark
	"’", "’’", // right single quotation mark
	"‚", "‚‚", // single low-9 quotation mark
	"‛", "‛‛", // single high-reversed-9 quotation mark
)

Covered by two cases in TestQuote: curly apostrophe (Bob’s Project → 'Bob’’s Project') and other smart single quotes (a‘b‚c‛d → 'a‘‘b‚‚c‛‛d'), each asserting bash and cmd leave them alone so the PowerShell-only scoping is pinned too. Bob’s Project is also in roundTripValues, so it goes through TestQuotePowerShell_RoundTrip and TestQuotePowerShell_RoundTripToNativeCommand on CI.

Two things worth recording:

Unverified by execution on my side — there is no pwsh or powershell on this machine (command -v finds neither), so the PowerShell round trip tests skipped locally and I'm taking your pwsh 7 result plus the tokenizer's documented character classes rather than reproducing it. about_Quoting_Rules lists exactly these four as single quotes, and the tokenizer's terminator check is on the character class rather than a match against the opening quote, which is why an ASCII-opened string is closed by a curly one; the doubling works because the escape rule is "the next character is also a single quote", not "the same character again". The pwsh round trip does run on CI (pwsh is on the ubuntu image, and lookShell fails rather than skips when CI is set), so the assertion is checked against a real parser there.

The smart double quotes — U+201C, U+201D, U+201E — are deliberately not in the replacer. They're in the same documented list, but the output is always a single quoted string and nothing terminates that except a single quote, so they're literal. isBare already forces quoting for them as non-ASCII.

= in the bare set breaks zsh, which this mode explicitly covers: a word starting with = undergoes zsh's =cmd expansion, so a bare =foo makes the whole command abort — verified with zsh 5.9: echo =foo → zsh: foo not found, exit 1. Mid-word = (a=b) is fine; it only needs the leading-position treatment, i.e. quote when the value begins with = (same category of hazard as ~, which you handled by exclusion).

Actioned in 1491852.

= stays in posixSafeChars — mid-word it is harmless, and taking it out would quote every --variable Name=Value needlessly. The leading-position case is handled where ~ is, in quotePosix:

// = is harmless in the middle of a word but a word which starts with one is subject
// to zsh's =cmd expansion, so `=foo` aborts the whole command with "foo not found".
if !strings.HasPrefix(value, "=") && isBare(value, posixSafeChars) {
	return value
}

Reproduced here on zsh 5.9 (macOS /bin/zsh --no-rcs -c), and the silent case is worse than the aborting one:

zsh  printf '%s' =foo   → "zsh:1: foo not found", exit 1
sh   printf '%s' =foo   → "=foo", exit 0
zsh  printf '%s' '=foo' → "=foo", exit 0
zsh  printf '%s' a=b    → "a=b", exit 0
zsh  printf '%s' =ls    → "/bin/ls", exit 0

=foo fails loudly, but a value that happens to name a real command — =ls — is substituted with its path and the command runs with the wrong argument, no error. That's the one that argues for quoting rather than documenting.

Covered by leading equals (=foo → '=foo', unquoted for PowerShell and cmd) and embedded equals (a=b stays bare everywhere) in TestQuote, and =foo is in roundTripValues. TestQuoteZsh_RoundTrip was added in the same commit and does run here — 32 subtests pass, including "=foo" and "a=b"; zsh is installed on the CI runner by pr-validation.yml so it runs there too.

An unrecognized --shell / OCTOPUS_SHELL value is silently swallowed: Parse fails, so Current falls through to detection and --shell powershel (typo) quietly emits detected-shell quoting — while config set Shell rejects the same value with an error. Since the user explicitly asked for a shell, consider validating the flag/env value (e.g. in root's PersistentPreRun) so an invalid choice errors, or at least warns, instead of being ignored.

Actioned in e89ef0d, then softened for the environment variable in 30e63a0. Doc comment corrected in 07cfce7.

The finding holds — Parse failing meant Current fell through to detection, so --shell powershel silently quoted for the detected shell. Validation now lives in root's PersistentPreRunE (which became PersistentPreRunE for it), but the two inputs are treated differently:

• --shell is a hard error: --shell: the provided value fish is not a valid shell, please use one of bash, powershell, cmd.
• OCTOPUS_SHELL warns on stderr and falls back to detection.

The split is the part I'd like you to confirm, because your comment asked for an error "or at least a warning" and I've landed on one of each. The reason for not failing on the environment variable: it's exported once, usually from a shell profile, and applies to every subsequent command, so rejecting it locks the user out of the whole CLI — including the octopus config set Shell they'd need to fix it. --shell doesn't have that problem: it was typed for one command and retyping it is the fix. The first version (e89ef0d) did fail on both; 30e63a0 walked the env var back for exactly that reason.

Covered by pkg/cmd/root/root_test.go: TestRoot_InvalidShellFlagIsRejected, TestRoot_ValidShellFlagIsAccepted, TestRoot_InvalidShellEnvIsWarnedAboutbutNotFatal, TestRoot_ValidShellEnvIsSilent, and TestRoot_ShellFlagSupersedesInvalidEnv (a good flag alongside a bad env var is silent, since the flag is the more specific signal). All pass.

Two residuals, both deliberate but say if you disagree:

1. The config file value is not checked in PersistentPreRunE at all, and Current ignores a bad one silently. config set Shell validates on the way in, so a bad value there can only come from hand editing the file. 07cfce7 fixes the code comment, which wrongly claimed the file value was warned about too.
2. The warning goes to stderr unconditionally, so a scripted --no-prompt run with a stale OCTOPUS_SHELL exported will emit it on every invocation. That seemed better than silence, but it is noise if anyone is in that state.

Open question: is one-error-one-warning the split you want, or would you rather OCTOPUS_SHELL also be fatal and accept the lockout (recoverable with env -u OCTOPUS_SHELL octopus ...)?

Two small gaps in the config surface:

1. There's no way back to auto-detect: octopus config set Shell "" fails validation, so once set, the only reset is hand-editing the config file. Allowing empty (the documented default) to clear the key would fix that.
2. config get's interactive promptMissing key list (pkg/cmd/config/get/get.go) wasn't given ConfigShell, so the new key appears in set's picker but not get's. (config get Shell by name still works, since IsValidKey goes through viper.AllKeys().)

Both actioned in 144f46b, with tests added in 24e693c.

1. Empty now clears the setting rather than failing validation, so there's a way back to auto-detection without hand editing the file:

case strings.ToLower(constants.ConfigShell):
	// empty is the documented default and clears the setting, putting the CLI back
	// to detecting the shell it is running under
	if value != "" {
		if err := shell.Validate(value); err != nil {
			return err
		}
	}
	localViper.Set(key, value)

It writes "shell": "" rather than removing the key — viper has no unset — but that's the same state as never having set it, since Current calls Parse(""), gets false, and falls through to Detect.

2. constants.ConfigShell is now in get's promptMissing list too, so the key appears in both pickers.

There were no tests over any of this — pkg/cmd/config/{get,list,set} had no test files at all — so 24e693c adds pkg/cmd/config/set/set_test.go. TestSetRun_Shell is a table over the Shell case: a name is stored, an alias is stored as typed, empty clears the setting asserts the written file has shell: "", and powershel / fish are rejected with the shell package's message. TestSetRun_OtherKeys covers the NoPrompt and default branches either side of the new case plus the invalid-key path, so reshaping the switch can't quietly drop them. The test points HOME (and AppData) at a temp dir and sets the global viper up against it, since IsValidKey deliberately reaches out to the global instance.

One thing I didn't do: get's and set's key lists are still two hand-maintained copies, which is how the omission you found happened in the first place — set gained ConfigShell and get didn't. A shared constants.ConfigKeys slice would make the next key a one-line change, but it touches both commands beyond what this PR needs, so I've left it. Want it in here or as a follow-up?